Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check SoftDeletes on HasOne or BelongsTo relations #1628

Merged
merged 4 commits into from
Mar 28, 2018
Merged

Check SoftDeletes on HasOne or BelongsTo relations #1628

merged 4 commits into from
Mar 28, 2018

Conversation

drahosistvan
Copy link
Contributor

Right now, if the related table uses SoftDeletes, it is not handled via the performJoin method.

yajra
yajra previously requested changes Mar 11, 2018
* @param string $type
*/
protected function performJoin($table, $foreign, $other, $type = 'left')
protected function performJoin($table, $foreign, $other, $deletedAt = false, $type = 'left')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put deletedAt as the last argument to prevent possible breaking change.

protected function checkSoftDeletesOnModel($model)
{
if (in_array('Illuminate\Database\Eloquent\SoftDeletes', class_uses($model))) {
return $model->getDeletedAtColumn();
Copy link
Owner

@yajra yajra Mar 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this return the fully qualified column name? Can't check the codes yet but PR looks good. Thanks!

@yajra
Copy link
Owner

yajra commented Mar 11, 2018

Sorry for late reply. The PR looks good. Will try to test this on actual project next week and possibly add a test case for this scenario.

@yajra yajra dismissed their stale review March 28, 2018 16:27

Seems to work fine.

@yajra yajra merged commit ae0ad02 into yajra:8.0 Mar 28, 2018
@yajra
Copy link
Owner

yajra commented Mar 28, 2018

Released on v8.4.2, thanks a lot! 🥂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants